chore: drop boto for signurlarity by natthan-pigoux · Pull Request #901 · DIRACGrid/diracx

natthan-pigoux · 2026-04-21T16:35:06Z

drop all boto dependencies for signurlarity
ensure azure-core[aio] dependency to ensure aiohttp
replace boto S3Client to signurlarity AsyncClient

Closes #712

read-the-docs-community · 2026-04-24T13:42:09Z

Documentation build overview

📚 diracx | 🛠️ Build #33199061 | 📁 Comparing 33ae657 against latest (79f2e22)

🔍 Preview build

152 files changed · + 6 added · ± 146 modified

+ Added

± Modified

natthan-pigoux · 2026-06-04T07:35:57Z

I just checked why test_upload_download_sandbox was failing with the new client, and I it seems that while uploading a second time the same sandbox , the s3_client.head_object (e.g.signurlarity client) raises:

Access denied to object 'diracAdmin/admin/admin/sha256:24946d6d432b6305e62d87b3becffcbdaa3cbdf1fc8e9a41c5ad3176f994884e.tar.zst' in bucket 'demo-sandboxes'. Check credentials and permissions.

Instead of a 200 response. That is why it uploads a second time the sandbox instead of updating access time.

Any idea why is that?

natthan-pigoux · 2026-06-04T11:30:12Z

Regarding the gubbins-client test. aiohttp dependency is missing now since it was first probably required through some boto dependencies.

In the default pixi env, aiohttp is present but not in the gubbins-client env. Pip shows me:

❯ pixi run -e default pip show aiohttp
Name: aiohttp
Version: 3.14.0
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: 
Author-email: 
License: Apache-2.0 AND MIT
Location: /home/npigoux/DIRAC/diracx/.pixi/envs/default/lib/python3.11/site-packages
Requires: aiohappyeyeballs, aiosignal, attrs, frozenlist, multidict, propcache, typing_extensions, yarl
Required-by:

❯ pixi run -e gubbins-client pip show aiohttp
WARNING: Package(s) not found: aiohttp

But I don't find any references of the aiohttp dependency in any diracx pyprojet.toml.

Where does it come from? and should I explicitly add the aiohttp in the gubbins-client dependencies.

natthan-pigoux · 2026-06-04T11:47:16Z

Where does it come from? and should I explicitly add the aiohttp in the gubbins-client dependencies.

It seems to come from signurlarity.

chaen · 2026-06-04T13:59:14Z

It seems to come from signurlarity.

I do not think we explicitly call aiohttp so we should not strictly depend on it. What I find strange is that the gubbins test fail while it does worker for the standard client

chaen · 2026-06-04T14:00:16Z

I just checked why test_upload_download_sandbox was failing with the new client, and I it seems that while uploading a second time the same sandbox , the s3_client.head_object (e.g.signurlarity client) raises:
Access denied to object 'diracAdmin/admin/admin/sha256:24946d6d432b6305e62d87b3becffcbdaa3cbdf1fc8e9a41c5ad3176f994884e.tar.zst' in bucket 'demo-sandboxes'. Check credentials and permissions.
Instead of a 200 response. That is why it uploads a second time the sandbox instead of updating access time.

Any idea why is that?

could that be some crappy config option like you had to update in the signularity tests ?

natthan-pigoux · 2026-06-10T08:35:21Z

do not think we explicitly call aiohttp so we should not strictly depend on it. What I find strange is that the gubbins test fail while it does worker for the standard client

So I checked the pixi env dependencies and I found that the aiohttp dependency is installed via opensearch-py[async] used in diracx-db. Which is not a diracx-client dependency and neither of gubbins-client.

Maybe only the gubbins-client test is failing and not the diracx-client test because it's calling a not mocked client code, which ends up calling azure core, which needs aoihttp... Could that be?

In addition, azure-core imports aiohttp but doesn't have it as a dependency...
(in site-packages/azure/core/pipeline/transport/_aiohttp.py) , c.f. https://github.com/Azure/azure-sdk-for-python/tree/main/sdk/core/azure-core#async-transport

natthan-pigoux · 2026-06-10T12:25:33Z

@chaen shall diracx-client depend on azure-core[aio] ? (c.f. https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/core/azure-core/pyproject.toml#L31)

aldbr · 2026-06-11T08:42:02Z

@chaen shall diracx-client depend on azure-core[aio] ? (c.f. https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/core/azure-core/pyproject.toml#L31)

ddev: yes

I just checked why test_upload_download_sandbox was failing with the new client, and I it seems that while uploading a second time the same sandbox , the s3_client.head_object (e.g.signurlarity client) raises:

Probably a configuration issue locally. @chrisburr will take a look at it

chrisburr · 2026-06-11T12:07:19Z

Access denied to object 'diracAdmin/admin/admin/sha256:24946d6d432b6305e62d87b3becffcbdaa3cbdf1fc8e9a41c5ad3176f994884e.tar.zst' in bucket 'demo-sandboxes'. Check credentials and permissions.

This is a signularity bug, fix is in DIRACGrid/signurlarity#39

Here is an LLM generated review of this PR. I checked the suggestions and I think I agree with all of them:

Expand to see details

🔴 Correctness bugs (diracx side)

1. `settings.py` — `except NoSuchBucketError` on `create_bucket` is dead code

try:
    await self._client.create_bucket(Bucket=self.bucket_name)
except NoSuchBucketError as e:
    raise ValueError(f"Failed to create bucket {self.bucket_name}") from e

Per signurlarity, create_bucket raises BucketAlreadyExistsError,
BucketAlreadyOwnedByYouError, or PresignError — never NoSuchBucketError. This
handler never fires: a real create failure propagates as a raw signurlarity exception and
bypasses the friendly ValueError. The old code caught the broad ClientError; the
equivalent is to catch PresignError (or the base SignurlarityError).

2. `s3.py` — `generate_presigned_download` is unused, copy-paste dead code

Lines 86–113 are a verbatim duplicate of generate_presigned_upload: same body, it calls
generate_presigned_post (a POST upload form, not a download URL), and the docstring still
says "for uploading a file to S3." It is not in __all__ and nothing references it — the
real download path in sandboxes.py:115 already uses
s3_client.generate_presigned_url("get_object", ...) directly. Remove it. (If a download
helper is genuinely wanted, it must call generate_presigned_url, not
generate_presigned_post.)

🟠 Behavioral changes worth a second look

3. `_s3_exists` now swallows all `PresignError` as "doesn't exist"

except (NoSuchBucketError, PresignError):
    # if e.response["Error"]["Code"] != "404":
    #     raise
    return False

head_object raises PresignError for every non-200 (404, 403, 400, 500, network) with
no status code exposed. The old code re-raised anything that wasn't a 404; now a transient /
auth / server error during s3_object_exists is silently reported as "object not uploaded"
(this is the exact mechanism that turns the 403 above into a spurious re-upload). Delete the
leftover commented-out # if ... raise, and consider an upstream signurlarity issue to expose
the HTTP status so 404 can be distinguished from 403/5xx.

4. `_s3_delete_chunk_with_retry` catches the wrong exception for retry

except NoSuchBucketError:
    ... retry with backoff ...

delete_objects raises NoSuchBucketError for a missing/forbidden bucket and PresignError
for transient signing/execution failures. The retry loop is meant for transient failures,
but those now arrive as PresignError and aren't caught — they propagate instead of retrying.
Conversely, retrying a genuinely-missing bucket with backoff is pointless. This looks
inverted: the retry path likely wants PresignError.

5. `s3_max_pool_connections` is now silently unused

The removed Config(max_pool_connections=…, signature_version="v4") is not replaced —
AsyncClient(**self.s3_client_kwargs) ignores it. The setting (and its docstring promising
more parallelism "during bulk sandbox deletion") is now dead. signurlarity uses httpx
underneath, so either wire this through to httpx.Limits or remove the setting and update the
docstring. (signature_version="v4" is safe to drop — signurlarity is always SigV4.)

🟡 Minor / style

Redundant import — settings.py:35 imports AsyncClient at runtime, then re-imports it
in the TYPE_CHECKING block (:42). Drop the TYPE_CHECKING copy.
azure-core → azure-core[aio] in diracx-client looks unrelated to dropping boto
(likely aiobotocore previously pulled in aiohttp transitively, which azure-core's async
transport relied on). Worth a one-line note in the PR description confirming it's intentional.
Unbounded pin — signurlarity >=0.3.0 with no upper bound on a 0.x library invites
surprise breakage. Consider >=0.3.0,<0.4.
Test nits — test_bucket calls objects.get("Contents", []) twice (hoist into a local);
the new await client.get(url,) in test_presigned_upload_moto has a stray trailing comma.

Details

natthan-pigoux · 2026-06-11T12:17:20Z

Thanks a lot @chrisburr for the fix and the review!

natthan-pigoux · 2026-06-11T13:41:15Z

About the review point 3. _s3_exists now swallows all PresignError as "doesn't exist", signurlarity head_object raises a PresignError on 404 response instead of a NoSuchBucketError. Is that on purpose? (c.f. DIRACGrid/signurlarity#41)

natthan-pigoux · 2026-06-11T15:26:10Z

Unbounded pin — signurlarity >=0.3.0 with no upper bound on a 0.x library invites
surprise breakage. Consider >=0.3.0,<0.4

Not sure we want to apply this, shall I?

natthan-pigoux marked this pull request as draft April 21, 2026 16:35

natthan-pigoux mentioned this pull request Apr 24, 2026

feat: add delete bucket method in client DIRACGrid/signurlarity#32

Merged

natthan-pigoux force-pushed the chore_from_boto_to_signurlarity branch from ac4a97b to ca6f734 Compare May 21, 2026 08:08

natthan-pigoux force-pushed the chore_from_boto_to_signurlarity branch from ca6f734 to d5ab9a7 Compare May 29, 2026 12:07

natthan-pigoux marked this pull request as ready for review June 4, 2026 07:38

natthan-pigoux force-pushed the chore_from_boto_to_signurlarity branch 2 times, most recently from 80ba1d2 to cf0a40f Compare June 4, 2026 11:33

aldbr requested a review from chaen June 4, 2026 11:50

natthan-pigoux force-pushed the chore_from_boto_to_signurlarity branch from cf0a40f to 809a590 Compare June 9, 2026 13:53

aldbr requested a review from chrisburr June 11, 2026 08:42

natthan-pigoux force-pushed the chore_from_boto_to_signurlarity branch 2 times, most recently from e47f0f2 to 462bdf9 Compare June 11, 2026 11:43

natthan-pigoux force-pushed the chore_from_boto_to_signurlarity branch from cbdb407 to 4c35ce8 Compare June 18, 2026 09:58

natthan-pigoux added 4 commits June 18, 2026 13:38

chore: drop boto for signurlarity

d47aa7e

chore: actually remove boto dependencies

45cc093

test: get the s3 object using presigned url

2877415

test: fix signurlarity exception match

d3547f3

natthan-pigoux and others added 14 commits June 18, 2026 13:38

test: fix minio tests using signurlarity

699839d

build: update signurlarity version with delete_bucket feature

7ba9650

build: diracx-client depends on azure-core[aio]

bf7c98e

fix: catch generic signurlarity error

f087d44

fix: handle PresignError 404

aaa3dee

refactor: remove unused generate_presigned_download

ac3d917

fix: retry delete chunk on PresignError

3bcf00a

fix: restore max pool connections

b6960da

fix: remove redondant import

c0d7b7b

refactor: avoid duplicating list objects contents

7cc6683

build: update signurlarity to v0.3.1

17cb708

chore: update pixi lock after rebase

14a1c59

fix: use signurlarity NoSuchKeyError exception and httpx2

b90786a

chore: restore pixi.lock from upstream/main and re-lock

33ae657

chrisburr force-pushed the chore_from_boto_to_signurlarity branch from 4a72ded to 33ae657 Compare June 18, 2026 11:40

chrisburr approved these changes Jun 18, 2026

View reviewed changes

chrisburr merged commit ddb0e8c into DIRACGrid:main Jun 18, 2026
29 checks passed

Uh oh!

Conversation

natthan-pigoux commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

read-the-docs-community Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation build overview

Uh oh!

natthan-pigoux commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natthan-pigoux commented Jun 4, 2026

Uh oh!

natthan-pigoux commented Jun 4, 2026

Uh oh!

chaen commented Jun 4, 2026

Uh oh!

chaen commented Jun 4, 2026

Uh oh!

natthan-pigoux commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natthan-pigoux commented Jun 10, 2026

Uh oh!

aldbr commented Jun 11, 2026

Uh oh!

chrisburr commented Jun 11, 2026

🔴 Correctness bugs (diracx side)

1. settings.py — except NoSuchBucketError on create_bucket is dead code

2. s3.py — generate_presigned_download is unused, copy-paste dead code

🟠 Behavioral changes worth a second look

3. _s3_exists now swallows all PresignError as "doesn't exist"

4. _s3_delete_chunk_with_retry catches the wrong exception for retry

5. s3_max_pool_connections is now silently unused

🟡 Minor / style

Uh oh!

natthan-pigoux commented Jun 11, 2026

Uh oh!

natthan-pigoux commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

natthan-pigoux commented Jun 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

natthan-pigoux commented Apr 21, 2026 •

edited

Loading

read-the-docs-community Bot commented Apr 24, 2026 •

edited

Loading

natthan-pigoux commented Jun 4, 2026 •

edited

Loading

natthan-pigoux commented Jun 10, 2026 •

edited

Loading

1. `settings.py` — `except NoSuchBucketError` on `create_bucket` is dead code

2. `s3.py` — `generate_presigned_download` is unused, copy-paste dead code

3. `_s3_exists` now swallows all `PresignError` as "doesn't exist"

4. `_s3_delete_chunk_with_retry` catches the wrong exception for retry

5. `s3_max_pool_connections` is now silently unused

natthan-pigoux commented Jun 11, 2026 •

edited

Loading